For consistency, could you do a find/replace and change all references of "Toolbar" to "ToolBar" and "toolbar" to "toolBar"?

Minor, but you could set a default value of alignment = LEFT_ALIGNMENT which would eliminate the need for the ternary expression.

I think we should add defaultProps for both rows and selectedRowIds and set these to an empty array [] by default. This way the logic which refers to their length property won't throw an error if these aren't provided. I think this makes sense because from a consumer's standpoint it should be valid to not provide these values if there are no rows or if none are selected.

I think we need a key prop on each of these elements. I think this goes for the other examples as well.

key is needed for these buttons.

I think we can drop the "Component" from names and just make this pager and actionComponent can become actions. Otherwise we're sort of implementing Hungarian notation.

Can we update this logic so that if itemsSelectedCount is 1 then the noun is singular ("item")?

I think this can just be pager, per an earlier comment.

We should probably migrate this into the UI Framework. Probably not necessarily as part of this PR though.

How about we combine noItemsPrompt and loading into a single prompt prop, and offload the logic for determining when to show which into the consumer? If a prompt is specified, it will be presented instead of the table. This gives the consumer some more control over exactly what the prompt looks like and what its content is.

BTW our style recommends early exiting so this would become:

renderPrompt() {
  if (this.state.isFetchingItems) {
    return <KuiListingTableLoadingPrompt />;
  }

  if (this.items.length === 0) {
    if (this.state.filter) {
      return <KuiListingTableNoMatchesPrompt />;
    }
    
    return <NoVisualizationsPrompt />;
  }

  return null;
}

This makes it a little easier (for me at least) to mentally map conditions to outcome.

Can we rename this to areAllRowsSelected for consistency?

This seems a little odd because these two conditions are implicitly mutually exclusive, but the second one will still be evaluated even if the first one is true. How about making it more explicit with a function?

export function KuiListingTableToolBarFooter({ pager, itemsSelectedCount }) {
  const renderText = () => {
    if (itemsSelectedCount === 1) {
      return "1 item selected";
    }

    return `${itemsSelectedCount} items selected`;
  };

  return (
    <KuiToolBarFooter>
      <KuiToolBarFooterSection>
        <KuiToolBarText>
          {renderText()}
        </KuiToolBarText>
      </KuiToolBarFooterSection>

      <KuiToolBarFooterSection>
        {pager}
      </KuiToolBarFooterSection>
    </KuiToolBarFooter>
  );
}

Yep, getting too fancy! :) It's more readable your way, will update.

I think the pattern I've been seeing more lately is to import Component from the React module instead.

import React, {
  Component,
} from 'react';

export class VisualizeListingTable extends Component {

Same here.

Does this need to be a method? Can we define this in the constructor instead?

This seems strange to me. If this is indeed a filter, we shouldn't need to make network requests when it changes, right? We just need to look at the superset of data, and filter it down to some subset?

unfortunately it does require a re-fetch because we don't grab all the items, we only grab the first 100. Eventually we'll implement paging, which will still require a re-fetch.

I'm guessing that's for performance issues? Have we explored doing a scroll with the ES search so we can continue searching in the background while results are available? It might not be a big deal, but it seems like we could show the initial 100, then quickly load the rest behind the scene?

One of our top notch community members explored this but ran into some complications with sorting and our index not having keyword fields for type and description. Read through #11415 to get the background. Something we want to do, but haven't had time to dig in deeper. We closed the issue because as a workaround we have a configurable setting to how many items can be retrieved.

Thanks! Makes sense!

What's the thinking behind this being data drive, instead of component driven? Like why not construct some component in the caller and pass it in directly?

This is what is passed in now:

return [
      {
        content: 'Title',
        onSort: this.sortByTitle,
        isSorted: this.state.sortedColumn === 'title',
        isSortAscending: this.sortableProperties.isAscendingByName('title'),
      },
      {
        content: 'Type',
        onSort: this.sortByType,
        isSorted: this.state.sortedColumn === 'type',
        isSortAscending: this.sortableProperties.isAscendingByName('type'),
      },
    ];

But what if we just did:

return [
  <KuiTableHeaderCell
       onSort: this.sortByTitle,
        isSorted: this.state.sortedColumn === 'title',
        isSortAscending: this.sortableProperties.isAscendingByName('title')
   >
      Title
   </KuiTableHeaderCell>
];

or something similar?

It feels we have too much unnecessary code around this header concept and a simpler solution might work out better. Thoughts?

This same question/concept applies to maybe some of the other props here, like rows.

I think I understand the inclination to separate the data from the presentational concerns/details, but it feels the coupling is pretty strong already so I'm not sure it's buying us much.

It was that way at first. It changed in this commit- f50f916
after a discussion with @cjcenizal. I like that it's more refactored in the data driven approach. Comment that initiated the change is here: f50f916#r142833708

Yea that makes sense. I'm not sure I love it still, but it's perfectly readable and still easy to understand so carry on!

For context, I've copy/pasted my original comment below. I was hoping that by encapsulating the row, header, and cell components within the table component, it would let the consumer use a simpler interface to specify their content. For example, if the rows' cells just needs to contain text, you can specify each row as an array of strings instead of needing to specify each cell component too. I was also hoping that that this would move some of the sorting logic into KuiListingTable.

That said, I'm not 100% sold on this approach and really welcome people's thoughts on it. I'd love it if we could improve these abstractions, but I'm also OK with reverting it if people find them too difficult to use.

I wonder if there's a way where we can create abstractions for the way headers and rows present data. If the interfaces of these abstractions were to be similar then I think that would be ergonomic for the consumer, since they're very similar components (they're both containers of the same number of cells).

I'm thinking of something like this:

// This component is responsible for instantiating `KuiTableRowCell` instances.
// It will iterate over each element in the cells array. You can specify a cell as a string,
// an element, a number, or a more complex configuration object. Depending
// on how the type is formatted, the values will either be passed as `children`
// or as various props to each `KuiTableRowCell`. 

const rows = this.items.map(item => (
  <KuiListingTableRow
    cells={[
      <a className="kuiLink" href={item.link}>item.name</a>,
      <div className={`kuiIcon kuiIcon--success ${item.icon}`}/>,
      item.date,
      {
        content: item.magnitude,
        align: 'right',
      },
    ]}
  />
));

// Same idea here.

const header = <KuiListingTableHeader
  cells={[{
    {
      content: 'Name',
      sortableId: 'name, // The presence of this property will present a column as sortable
    },
    'Icon',
    {
      content: 'Date',
      sortableId: 'date,
    }, {
      content: 'Magnitude',
      align: 'right',
      sortableId: 'magnitude',
    }
  }]}
/>

// And then maybe we could leverage `children' and the table would become this:

<KuiListingTable
  pager={this.renderPager()}
  toolbarActions={this.renderToolbarActions()}
  selectedRowIds={this.state.selectedRowIds}
  header={header}
  onFilter={() => {}}
  filter=""
  onItemSelectionChanged={this.onItemSelectionChanged}
>
  {rows}
</KuiListingTable>

What do you think? Does this sound crazy?

Right, as @cjcenizal said, you can still pass an array of nodes or strings for the header and rows, it's just the internal part, not to include the KuiTableRowCell part. But this means we have to pass in sort information and align information sometimes, for the props that go on that outer element.

I'm also flexible if we find a better approach. For now I'll go ahead and submit once this passes, and we can update when/if we find some better abstractions or patterns. Perhaps once we add in an HOC and refactor the sorting logic, we'll find a better way to handle it.

It's easy to follow and totally makes sense. I can't articulate my thoughts against it that well, which could easily indicate they aren't grounded in anything useful. Please continue as is and I'm sure we'll find another time to rethink it and maybe have some more useful conversations (on my side at least!)

Should it also show one with sorting? There's also some mention of innerTable, should we show that here too?

The innerTable isn't it's own component. We could move it out, but for now, it shouldn't get it's own section in the docs view. I will give one of the headers sort attributes.